Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typescript declarations in cts and mts formats #4061

Closed
wants to merge 3 commits into from

Conversation

chrmod
Copy link
Member

@chrmod chrmod commented Jul 2, 2024

This draft updates one package for purpose of demonstrating the direction. If accepted, all packages will be updated in this PR.

Fixes #4054

Typescript assumes that type delacrations for commonjs files will have .cts file extension.

To achieve that, we had to:

  • remove tsc from the build step
  • let rollup compile Typescript with @rollup/plugin-typescript
  • find a workaround rollup not being able to emit declarations
  • copy declarations entry point to new file extension with rollup-plugin-copy

As a byproduct of the workaround, the /dist file structure was flatten and now all esm, commonjs and declarations files are next to each other. One positive outcome of that change is that declarations can be automatically picked up without package.json referencing them explicitly. We have however kept those references for clarity.

New flatten file structure looks like this:

dist/types
dist/types/adblocker.d.cts
dist/types/adblocker.d.ts
dist/types/src
dist/types/src/parse.d.ts
dist/types/src/extended.d.ts
dist/types/src/types.d.ts
dist/types/src/eval.d.ts
dist/adblocker.cjs
dist/adblocker.js
dist/adblocker.umd.min.js
dist/adblocker.umd.min.js.map
dist/adblocker.cjs.map
dist/adblocker.js.map
dist/src
dist/src/extended.js
dist/src/types.js
dist/src/types.js.map
dist/src/eval.cjs.map
dist/src/eval.js.map
dist/src/types.cjs
dist/src/extended.cjs
dist/src/parse.js.map
dist/src/parse.cjs
dist/src/parse.js
dist/src/eval.js
dist/src/eval.cjs
dist/src/extended.cjs.map
dist/src/extended.js.map
dist/src/types.cjs.map
dist/src/parse.cjs.map

@chrmod chrmod force-pushed the ts-declarations-in-cts-and-mts branch from 8c03131 to 33f8dee Compare July 2, 2024 14:10
@chrmod chrmod added the PR: Bug Fix 🐛 Increment patch version when merged label Jul 2, 2024
@seia-soto
Copy link
Member

seia-soto commented Jul 3, 2024

Leaving a note from the internal discussion for a future reference:

The use of cjs file extension will not break anything even in legacy Node.JS versions.

The automatic module searching in Node.JS versions prior to the ESM release is not the problem as we specify the full filename in require. For example, requiring a module with path with full extension: e.g. ./module.cjs will not break anything.

Only type entry point filename needs to be changed to .d.cts not the whole type declarations.

This is because we didn't specify the file extension in import calls of the type declarations. This will allow typescript compiler to resolve type declarations in sub directories dynamically from the different entrypoints in the same directory. If the typescript compiler emitted the type declaration in full filename with extension, this workaround won't work anymore: e.g. import type { A } from './sub-type.d.cts'. Thus, in the future, we may want to bundle the types again if typescript compiler changes. However, resolving type modules for typescript compiler is not the problem at the current timespan, so the current setup is the optimal in the output size.

Also, later, I found that this problem is already addressed from the typescript release note:

Attempting to use a single .d.ts file to type both an ES module entrypoint and a CommonJS entrypoint will cause TypeScript to think only one of those entrypoints exists, causing compiler errors for users of the package.
-- https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-7.html#packagejson-exports-imports-and-self-referencing

For the source files, we specify the full name of files. Also, this doesn't make sense due to the module system difference.

For the resolution steps in depth, please see the following two:

Not using m prefix (e.g. mjs and mts) for all defaults.

At this timespan, the whole community is already settled to ESM as a preferred default module system for Node.JS. Thus using js instead of mjs for ESM styles makes sense, instead of flagging ESM as something different by using different file extension. Also see: nodejs/modules#293

The types and modules fields in package.json are only for backward-compatibility and will follow the above file extension stance.

The types (for legacy typescript versions) and module (is the bundlers standard not the Node standard) fields are deprecated since typescript version 4.7 to integrate Node community with ESM better. Therefore, these fields are considered as legacies and will follow the above file extension stances. Also, see the following example from typescript 4.7 release note:

// package.json
{
    "name": "my-package",
    "type": "module",
    "exports": {
        ".": {
            // Entry-point for `import "my-package"` in ESM
            "import": {
                // Where TypeScript will look.
                "types": "./types/esm/index.d.ts",
                // Where Node.js will look.
                "default": "./esm/index.js"
            },
            // Entry-point for `require("my-package") in CJS
            "require": {
                // Where TypeScript will look.
                "types": "./types/commonjs/index.d.cts",
                // Where Node.js will look.
                "default": "./commonjs/index.cjs"
            },
        }
    },
    // Fall-back for older versions of TypeScript
    "types": "./types/index.d.ts",
    // CJS fall-back for older versions of Node.js
    "main": "./commonjs/index.cjs"
}

* chore: update gitignore

* feat(adblocker): update build config

* feat(adblocker-content): update build config

* chore(adblocker-content): sync declarationDir of tsconfig

* feat(adblocker-electron): update build config

* feat(adblocker-electron-preload): update build config

* feat(adblocker-playwright): update build config

* feat(adblocker-puppeteer): update build config

* feat(adblocker-webextension): update build config

* feat(adblocker-webextension-cosmetics): update build config

* chore: update npmignore

* chore: clean up

- drop "declarationDir" from tsconfig which will cause conflict with @rollup/plugin-typescript
- properly specify file location with prefix of "./"
- fix up some file orders
- fix the declaration directory to "./dist/types"
- create npmignore unless exists

* feat(adblocker-webextension-example): update build config

* chore(adblocker-electron-example): remove accidental build output

* feat: update tsconfig for examples

commonjs is no longer recommended, switching to nodenext as we now properly support cjs modules
@seia-soto
Copy link
Member

seia-soto commented Jul 10, 2024

Just a note: a command to check if cts generation was successful: rm -rf ./packages/*/dist && yarn build && find ./packages -name "*.d.cts"

This was referenced Jul 10, 2024
@chrmod
Copy link
Member Author

chrmod commented Jul 12, 2024

closing in favour of #4079

@chrmod chrmod closed this Jul 12, 2024
@chrmod chrmod deleted the ts-declarations-in-cts-and-mts branch October 17, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛 Increment patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Typescript] CommonJS module support
2 participants